standardize file copy to output#372
Open
drbergman wants to merge 2 commits intoMathCancer:developmentfrom
Open
standardize file copy to output#372drbergman wants to merge 2 commits intoMathCancer:developmentfrom
drbergman wants to merge 2 commits intoMathCancer:developmentfrom
Conversation
drbergman
force-pushed
the
copy-config-in-core
branch
from
3a2795f to
0c7b54b
Compare
drbergman
commented
Apr 29, 2025
Collaborator
Author
drbergman
left a comment
drbergman
left a comment
There was a problem hiding this comment.
I have reviewed all the updated main.cpp's and they have all been properly updated. The episodes project has a unique structure, however, and is unchanged. Again, all this should mean is repeated copying for that project, but nothing excessive.
drbergman
changed the title
- default names: - config: PhysiCell_settings.xml - rules: cell_rules.csv - ic cells: cells.csv - ic substrates: substrates.csv or substrates.mat
drbergman
force-pushed
the
copy-config-in-core
branch
from
d853fa0 to
ae0c2b7
Compare
There was a problem hiding this comment.
Pull Request Overview
Standardize how configuration, microenvironment, geometry, and rule files are copied to the output directory by centralizing operations in copy_file_to_output with an optional default_basename parameter.
- Removed redundant manual
cpcommands from all sample projectmain.cppfiles. - Updated core utilities to accept a
default_basenameand invoke a second copy when needed. - Applied
copy_file_to_outputuniformly in modules for settings, microenvironment, cell data, and rules.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sample_projects/worm/main.cpp | Removed manual config copy |
| sample_projects/virus_macrophage/main.cpp | Removed manual config copy |
| sample_projects/template/main.cpp | Removed manual config copy |
| sample_projects/rules_sample/main.cpp | Removed manual config copy |
| sample_projects/pred_prey_farmer/main.cpp | Removed manual config copy |
| sample_projects/physimess/main.cpp | Removed manual config copy |
| sample_projects/mechano/main.cpp | Removed manual config copy |
| sample_projects/interactions/main.cpp | Removed manual config copy |
| sample_projects/immune_function/main.cpp | Removed manual config copy |
| sample_projects/heterogeneity/main.cpp | Removed manual config copy |
| sample_projects/custom_division/main.cpp | Removed manual config copy |
| sample_projects/cancer_immune/main-cancer_immune_3D.cpp | Removed manual config copy |
| sample_projects/cancer_biorobots/main.cpp | Removed manual config copy |
| sample_projects/biorobots/main.cpp | Removed manual config copy |
| sample_projects/asymmetric_division/main.cpp | Removed manual config copy |
| modules/PhysiCell_settings.cpp | Added default_basename arguments to copy settings XML |
| modules/PhysiCell_geometry.cpp | Added default_basename for cell CSV copy |
| core/PhysiCell_utilities.h | Extended copy_file_to_output signature with default |
| core/PhysiCell_utilities.cpp | Implemented double-copy logic with default_basename |
| core/PhysiCell_rules.cpp | Applied default_basename for rule file copy |
Comments suppressed due to low confidence (2)
core/PhysiCell_utilities.cpp:383
- Using
sprintfandsystem()with unvalidated filenames can lead to shell injection and portability issues; consider replacing this withstd::filesystem::copyor another safe API.
sprintf(copy_command, "cp %s %s", filename.c_str(), output_filename.c_str());
core/PhysiCell_utilities.h:114
- Add unit tests for
copy_file_to_outputcovering both the single-copy and double-copy (with non-emptydefault_basename) paths to verify correct behavior.
void copy_file_to_output(const std::string &filename, const std::string &default_basename = "");
|
|
||
| // copy the file to the output folder with the default basename | ||
| std::string default_output_filename = PhysiCell_settings.folder + "/" + default_basename; | ||
| sprintf(copy_command, "cp %s %s", filename.c_str(), default_output_filename.c_str()); |
There was a problem hiding this comment.
Similarly, this second sprintf + system() call risks command injection and may fail on non-Unix platforms; use a filesystem-native copy routine instead.
| // copy the file to the output folder with the default basename | ||
| std::string default_output_filename = PhysiCell_settings.folder + "/" + default_basename; | ||
| sprintf(copy_command, "cp %s %s", filename.c_str(), default_output_filename.c_str()); | ||
| (void)system(copy_command); // make it explicit that we are ignoring the return value |
There was a problem hiding this comment.
The return value of system() is ignored here, so failures go unnoticed; consider capturing and handling the result or throwing an exception on error.
Suggested change
| (void)system(copy_command); // make it explicit that we are ignoring the return value | |
| int ret_code = system(copy_command); | |
| if (ret_code != 0) { | |
| std::cerr << "Error: Failed to execute command: " << copy_command << std::endl; | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Standardize how all files are copied to output. This serves several purposes:
main.cppdefault_basenameargument incopy_file_to_outputensures standard file naming in outputNote: If a user retains the copy command in their
main.cpp, this will just mean it is copied twice which will not cause errors.